Add support for user-scoped extension settings #13748
Conversation
Summary of ChangesHello @chrstnb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of extension settings by introducing user and workspace-level configuration options. This allows developers to maintain global preferences while also enabling project-specific overrides, improving the adaptability of extensions to diverse development environments. The changes include updates to CLI commands and underlying logic to support this new scoping behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for user-scoped and workspace-scoped extension settings, which is a great enhancement. The implementation correctly introduces the concept of scopes for both file-based settings and sensitive secrets in the keychain, and correctly handles the precedence of workspace settings over user settings. However, I've found a critical issue in the updateSetting function that could lead to data loss in users' .env files by deleting any variables not managed by the extension. I've also identified a couple of significant issues in the test suite where the tests are not correctly validating the implementation, including one that fails to catch the data loss bug, and another that doesn't properly test the isolation of secrets between scopes. Addressing these issues is crucial before merging.
| it('should merge user and workspace settings, with workspace taking precedence', async () => { | ||
| // User settings | ||
| const userEnvPath = path.join(extensionDir, EXTENSION_SETTINGS_FILENAME); | ||
| await fsPromises.writeFile( | ||
| userEnvPath, | ||
| 'VAR1=user-value1\nVAR3=user-value3', | ||
| ); | ||
| const userKeychain = new KeychainTokenStorage( | ||
| `Gemini CLI Extensions test-ext ${extensionId}`, | ||
| ); | ||
| await userKeychain.setSecret('VAR2', 'user-secret2'); | ||
|
|
||
| // Workspace settings | ||
| const workspaceEnvPath = path.join( | ||
| tempWorkspaceDir, | ||
| EXTENSION_SETTINGS_FILENAME, | ||
| ); | ||
| await fsPromises.writeFile(workspaceEnvPath, 'VAR1=workspace-value1'); | ||
| const workspaceKeychain = new KeychainTokenStorage( | ||
| `Gemini CLI Extensions test-ext ${extensionId} ${tempWorkspaceDir}`, | ||
| ); | ||
| await workspaceKeychain.setSecret('VAR2', 'workspace-secret2'); | ||
|
|
||
| const contents = await getEnvContents(config, extensionId); | ||
|
|
||
| expect(contents).toEqual({ | ||
| VAR1: 'workspace-value1', | ||
| VAR2: 'workspace-secret2', | ||
| VAR3: 'user-value3', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test case is intended to validate that workspace settings take precedence over user settings, including for sensitive values stored in the keychain. However, the underlying mock for KeychainTokenStorage is flawed, as it uses a single, shared data store for all instances, regardless of the serviceName that differentiates user and workspace keychains. This means the test doesn't correctly verify the isolation of secrets. The test passes coincidentally because the workspace secret is set after the user secret, overwriting it in the shared mock data. To fix this, the KeychainTokenStorage mock should be updated to partition its data based on the serviceName to ensure user and workspace secrets are stored and retrieved independently.
| it('should leave existing, unmanaged .env variables intact when updating in WORKSPACE scope', async () => { | ||
| // Setup a pre-existing .env file in the workspace with unmanaged variables | ||
| const workspaceEnvPath = path.join(tempWorkspaceDir, '.env'); | ||
| const originalEnvContent = | ||
| 'PROJECT_VAR_1=value_1\nPROJECT_VAR_2=value_2\nVAR1=original-value'; // VAR1 is managed by extension | ||
| await fsPromises.writeFile(workspaceEnvPath, originalEnvContent); | ||
|
|
||
| it('should wrap values with spaces in quotes', async () => { | ||
| mockRequestSetting.mockResolvedValue('a value with spaces'); | ||
| // Simulate updating an extension-managed non-sensitive setting | ||
| mockRequestSetting.mockResolvedValue('updated-value'); | ||
| await updateSetting( | ||
| config, | ||
| '12345', | ||
| 'VAR1', | ||
| mockRequestSetting, | ||
| ExtensionSettingScope.WORKSPACE, | ||
| ); | ||
|
|
||
| await updateSetting(config, '12345', 'VAR1', mockRequestSetting); | ||
| // Read the .env file after update | ||
| const actualContent = await fsPromises.readFile( | ||
| workspaceEnvPath, | ||
| 'utf-8', | ||
| ); | ||
|
|
||
| const expectedEnvPath = path.join(extensionDir, '.env'); | ||
| const actualContent = await fsPromises.readFile(expectedEnvPath, 'utf-8'); | ||
| expect(actualContent).toContain('VAR1="a value with spaces"'); | ||
| // Assert that original variables are intact and extension variable is updated | ||
| expect(actualContent).toContain('PROJECT_VAR_1=value_1'); | ||
| expect(actualContent).toContain('PROJECT_VAR_2=value_2'); | ||
| expect(actualContent).toContain('VAR1=updated-value'); | ||
|
|
||
| // Ensure no other unexpected changes or deletions | ||
| const lines = actualContent.split('\n').filter((line) => line.length > 0); | ||
| expect(lines).toHaveLength(3); // Should only have the three variables | ||
| }); |
There was a problem hiding this comment.
This test case is intended to verify that unmanaged variables in a .env file are preserved when an extension setting is updated. However, this test is flawed as it should be failing with the current implementation of updateSetting. The updateSetting function incorrectly overwrites the entire .env file, which would delete the PROJECT_VAR_1 and PROJECT_VAR_2 variables. The test's assertions that these variables are still present should fail. This indicates a potential issue with the test setup or how its assertions are being evaluated. After fixing the data loss bug in updateSetting, this test will be essential for verifying the correct behavior.
1 participant
Summary
Allow users to configure settings at either the workspace or user-level, with user-level being the default. If a user wants to set a per-workspace setting, it will be set in cwd/.env if non-sensitive and in keychainExtensionPath+cwd if sensitive. Workspace-level settings will take precedence over user-level settings.
Details
Related Issues
How to Validate
Pre-Merge Checklist